Add reference timeout parameter to controllers templates #111

Robotgir · 2023-03-14T15:56:22Z

shall include all changes related to reference timeout

Robotgir · 2023-03-23T09:41:37Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

@@ -139,6 +143,30 @@ controller_interface::CallbackReturn DummyClassName::on_configure(
  return controller_interface::CallbackReturn::SUCCESS;
 }

+void DummyClassName::reference_callback(const std::shared_ptr<ControllerReferenceMsg> msg)


saw that ref_callback is called in different order in standard and chainable controller, hence adjusting it to be consistent with standard controller

in the chainable controller,
this callback is not used at all. I mean, you can call it, but nothing will happen.

sorry for the confusion i should have written "ref_callback is defined in different order in standard and chain-able controller classes, hence adjusting it to be consistent with standard controller"

its trivial change, just changed the order

Robotgir · 2023-03-23T09:45:06Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


saw that usage of "/reference" is inconsistent in the current master version changed to "/reference" completely in a different PR
#106

Robotgir · 2023-03-23T10:00:41Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

@@ -192,7 +195,7 @@ TEST_F(DummyClassNameTest, test_update_logic_fast)

  EXPECT_EQ(*(controller_->control_mode_.readFromRT()), control_mode_type::FAST);
  EXPECT_EQ(joint_command_values_[STATE_MY_ITFS], TEST_DISPLACEMENT);
-  EXPECT_TRUE(std::isnan((*(controller_->input_ref_.readFromRT()))->displacements[0]));
+  EXPECT_EQ((*(controller_->input_ref_.readFromRT()))->displacements[0], TEST_DISPLACEMENT);  


i just adjusted this to work here, but wanted to get confirmation from you. I see that in mecanum we set reference msg to nan when ref_timeout = 0 and for too old msg (age_of_last_command > ref_timeout_)

in this particular test we have testing update fast logic where age_of_last_command <= ref_timeout_

and more over this test will be over written in the PR #106
with a different name with similar logic test

Robotgir · 2023-03-23T10:05:00Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

@@ -366,6 +372,166 @@ TEST_F(DummyClassNameTest, receive_message_and_publish_updated_status)
  ASSERT_EQ(msg.set_point, 0.45);
 }

+// when too old msg is sent expect reference msg reset


added ref_timeout related tests with agreed naming convention

destogl · 2023-03-27T09:44:57Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


Suggested change

"~/commands", subscribers_qos,

"~/reference", subscribers_qos,

ok change ing it to /reference everywhere

destogl · 2023-03-27T09:46:10Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

@@ -139,6 +143,30 @@ controller_interface::CallbackReturn DummyClassName::on_configure(
  return controller_interface::CallbackReturn::SUCCESS;
 }

+void DummyClassName::reference_callback(const std::shared_ptr<ControllerReferenceMsg> msg)


in the chainable controller,
this callback is not used at all. I mean, you can call it, but nothing will happen.

destogl

Please use reference instead of command.
One test is not fully implemented when testing only a single call.
Why is there repetition of the logic in both the update methods? Only method that gets things from callback should have the logic.

destogl · 2023-03-27T09:49:21Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+  const auto age_of_last_command = get_node()->now() - msg->header.stamp;
+  if (msg->joint_names.size() == params_.joints.size()) {
+    if (ref_timeout_ == rclcpp::Duration::from_seconds(0) || age_of_last_command <= ref_timeout_) {
+      input_ref_.writeFromNonRT(msg);


Should we not have here default value of the header is timestamp is "0"?

you are referring to this handling of case if header.stamp ==0 right?

this is included in the improvements from mecanum PR, you can see the line in the below link, should i also implement this in this ref_timeout PR?

https://github.com/StoglRobotics/ros_team_workspace/pull/106/files#diff-6e6a1012850b4b6040c5339b00ffad68f776d9edb72dbbe3e1d84482d52c4081R189-R197

destogl · 2023-03-27T10:22:06Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+  auto current_ref = input_ref_.readFromRT();
+  const auto age_of_last_command = get_node()->now() - (*current_ref)->header.stamp;


why is this here? This should be in the update reference from subscribers only. Please adjust

destogl · 2023-03-27T10:22:28Z

templates/ros2_control/controller/dummy_chainable_controller.cpp

+    // send message only if there is no timeout
+    if (age_of_last_command <= ref_timeout_ || ref_timeout_ == rclcpp::Duration::from_seconds(0)) {
+      if (!std::isnan(reference_interfaces_[i])) {
+        if (*(control_mode_.readFromRT()) == control_mode_type::SLOW) {
+          reference_interfaces_[i] /= 2;
+        }
+        command_interfaces_[i].set_value(reference_interfaces_[i]);
+        if (ref_timeout_ == rclcpp::Duration::from_seconds(0)) {
+          reference_interfaces_[i] = std::numeric_limits<double>::quiet_NaN();
+        }
      }
-      command_interfaces_[i].set_value(reference_interfaces_[i]);
-
-      reference_interfaces_[i] = std::numeric_limits<double>::quiet_NaN();
+    } else {
+      command_interfaces_[i].set_value(0.0);
+      (*current_ref)->displacements[i] = std::numeric_limits<double>::quiet_NaN();


Why is the code here repeated?

destogl · 2023-03-27T10:22:46Z

templates/ros2_control/controller/dummy_controller.cpp

  // Reference Subscriber
  ref_subscriber_ = get_node()->create_subscription<ControllerReferenceMsg>(
-    "~/reference", subscribers_qos,
+    "~/commands", subscribers_qos,


Suggested change

"~/commands", subscribers_qos,

"~/reference", subscribers_qos,

destogl · 2023-03-27T10:23:53Z

templates/ros2_control/controller/dummy_controller.cpp

-      command_interfaces_[i].set_value((*current_ref)->displacements[i]);
-
+    } else {
+      command_interfaces_[i].set_value(0.0);


here under is missing resetting of refrence interfaces...

this is standard controller and in my understanding we dont have reference_interfaces in standard controller. Please correct me if i am wrong

destogl · 2023-03-27T10:31:08Z

templates/ros2_control/controller/test_dummy_chainable_controller.cpp

+  EXPECT_FALSE(std::isnan((*(controller_->input_ref_.readFromNonRT()))->duration));
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->displacements[0], 0.45);
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->velocities[0], 0.0);
+  EXPECT_EQ((*(controller_->input_ref_.readFromNonRT()))->duration, 1.25);


you should call this also a second time to be sure that this works only once - not you can not confirm that.

extended the test here like this but have question,
it works when setting the ref_msg to nan like done here

but instead of individually setting them like that i wanted to just call
reset_controller_reference_msg(*(input_ref_.readFromRT()), state_joints_, get_node());

but this is not working, also made sure that the fun is being called but the msg is not being reset. is the way i am inputing the ref_msg wrong? like this "*(input_ref_.readFromRT())".

…lback_expect_reference_msg_being_used_only_once and extending its relavant code in controllers

mergify · 2023-05-12T07:06:52Z

This pull request is in conflict. Could you fix it @Robotgir?

mergify · 2023-08-11T12:34:26Z

This pull request is in conflict. Could you fix it @Robotgir?

mergify · 2023-10-09T19:09:38Z

This pull request is in conflict. Could you fix it @Robotgir?

mergify · 2023-12-12T09:51:07Z

This pull request is in conflict. Could you fix it @Robotgir?

mergify · 2024-02-01T16:40:49Z

This pull request is in conflict. Could you fix it @Robotgir?

GiridharBukka added 9 commits March 14, 2023 12:25

added ref_timeout a parameter

df36359

added ref_timeout related

0cef88d

both ref_time related and improvements from mecanum controller

62ec6f3

correction

8b5c1c9

added tests (new names) onl ref_timeout specific

77e1b53

chainable added missing

27014c9

regular controller added missing

64c8f1d

regular controller added missing tests

e2cc41e

corrections

3f27a6e

destogl requested review from destogl and muritane March 21, 2023 16:43

GiridharBukka added 3 commits March 23, 2023 08:12

all tests pass except load_controller_test

524e334

all tests pass except for load_controller in chaiable controller

508ce0e

correction

40aef82

Robotgir commented Mar 23, 2023

View reviewed changes

GiridharBukka added 2 commits March 23, 2023 11:08

corrections

72605dc

ran pre.commit

c02dd31

destogl reviewed Mar 27, 2023

View reviewed changes

destogl requested changes Mar 27, 2023

View reviewed changes

GiridharBukka added 3 commits March 29, 2023 18:41

/commands to /reference

0fb82f6

removed duplicate code for age_of_last_command

74b4d11

finishing the incomplete test when_ref_timeout_zero_for_reference_cal…

9692b17

…lback_expect_reference_msg_being_used_only_once and extending its relavant code in controllers

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add reference timeout parameter to controllers templates #111

Add reference timeout parameter to controllers templates #111

Robotgir commented Mar 14, 2023

Robotgir Mar 23, 2023

destogl Mar 27, 2023

Robotgir Mar 29, 2023

Robotgir Mar 23, 2023

Robotgir Mar 23, 2023

Robotgir Mar 23, 2023

destogl Mar 27, 2023

Robotgir Mar 29, 2023

destogl Mar 27, 2023

destogl left a comment •

edited

Loading

destogl Mar 27, 2023

Robotgir Mar 29, 2023

destogl Mar 27, 2023

destogl Mar 27, 2023

destogl Mar 27, 2023

destogl Mar 27, 2023

Robotgir Mar 29, 2023

destogl Mar 27, 2023

Robotgir Mar 29, 2023

mergify bot commented May 12, 2023

mergify bot commented Aug 11, 2023

mergify bot commented Oct 9, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Feb 1, 2024

	"~/commands", subscribers_qos,
	"~/reference", subscribers_qos,

		auto current_ref = input_ref_.readFromRT();
		const auto age_of_last_command = get_node()->now() - (*current_ref)->header.stamp;

Add reference timeout parameter to controllers templates #111

Are you sure you want to change the base?

Add reference timeout parameter to controllers templates #111

Conversation

Robotgir commented Mar 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

destogl left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mergify bot commented May 12, 2023

mergify bot commented Aug 11, 2023

mergify bot commented Oct 9, 2023

mergify bot commented Dec 12, 2023

mergify bot commented Feb 1, 2024

destogl left a comment •

edited

Loading